Skip to content

Conversation

@jinge90
Copy link
Contributor

@jinge90 jinge90 commented Mar 10, 2025

No description provided.

@jinge90 jinge90 requested review from a team as code owners March 10, 2025 08:49
@jinge90 jinge90 requested review from kbenzie and npmiller March 10, 2025 08:49
@jinge90 jinge90 requested a review from steffenlarsen March 10, 2025 08:50
@jinge90 jinge90 marked this pull request as draft March 10, 2025 08:50
@jinge90
Copy link
Contributor Author

jinge90 commented Mar 10, 2025

Hi, @steffenlarsen and @kbenzie
I drafted a PR to solve #17084 , could you help review to see whether we can proceed with this PR?
Thanks very much!

Signed-off-by: jinge90 <[email protected]>
Comment on lines 1042 to 1046
if ((Device->ZeDeviceProperties->deviceId & 0xfff) == 0x201 ||
(Device->ZeDeviceProperties->deviceId & 0xff0) == 0xbd0)
return ReturnValue(true);
else
return ReturnValue(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ((Device->ZeDeviceProperties->deviceId & 0xfff) == 0x201 ||
(Device->ZeDeviceProperties->deviceId & 0xff0) == 0xbd0)
return ReturnValue(true);
else
return ReturnValue(false);
return ReturnValue(
(Device->ZeDeviceProperties->deviceId & 0xfff) == 0x201 ||
(Device->ZeDeviceProperties->deviceId & 0xff0) == 0xbd0));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be in line with the changes here? https://github.com/intel/llvm/pull/17364/files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
Thanks for pointing out this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @steffenlarsen
Done.
Thanks very much.

/// point
UR_DEVICE_INFO_KERNEL_SET_SPECIALIZATION_CONSTANTS = 106,
/// [::ur_bool_t] support for native bfloat16 conversions
UR_DEVICE_INFO_BFLOAT16_CONVERSIONS_INTEL = 107,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this is added here and not as 122 after UR_DEVICE_INFO_PROGRAM_SET_SPECIALIZATION_CONSTANTS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @steffenlarsen
At the beginning, I just used '122' but the pre-ci unified_runtime source check failed and asked to changed to '107', I don't know why the check asked so, just followed it.
Thanks very much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the new etor is added part way through the yaml array.

Comment on lines 420 to 421
- name: BFLOAT16_CONVERSIONS_INTEL
desc: "[$x_bool_t] support for native bfloat16 conversions"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid rewriting all of the enum values, move this new entry to the end of the array, i.e. after line 451.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this to the bottom and run the generate again please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @kbenzie
Fixed.
Thanks very much.

/// point
UR_DEVICE_INFO_KERNEL_SET_SPECIALIZATION_CONSTANTS = 106,
/// [::ur_bool_t] support for native bfloat16 conversions
UR_DEVICE_INFO_BFLOAT16_CONVERSIONS_INTEL = 107,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because the new etor is added part way through the yaml array.

@jinge90 jinge90 marked this pull request as ready for review March 12, 2025 06:34
Copy link
Contributor

@npmiller npmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CUDA/HIP changes LGTM

@jinge90 jinge90 requested a review from steffenlarsen March 12, 2025 10:17
Comment on lines 1761 to 1763
TEST_P(urDeviceGetInfoTest, SuccessBFloat16) {
size_t property_size = 0;
const ur_device_info_t property_name = UR_DEVICE_INFO_BFLOAT16;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for the old deprecated bfloat16 query, there's a pr open to remove it https://github.com/intel/llvm/pull/17053/files#diff-4ad6c68f3c3e27ec6c124dab1d1c2cd28dc6bba6f4e0960f44eec79b3c0c1c98

for testing if you could just copy this function but rename the test and update property_name that'd be ideal, like this:

TEST_P(urDeviceGetInfoTest, SuccessBFloat16Conversions) {
  size_t property_size = 0;
  const ur_device_info_t property_name = UR_DEVICE_INFO_BFLOAT16_CONVERSIONS_INTEL;

desc: "[$x_bool_t] support the $xProgramSetSpecializationConstants entry point"
- name: USE_NATIVE_ASSERT
desc: "[$x_bool_t] return true if the device has a native assert implementation."
- name: BFLOAT16_CONVERSIONS_INTEL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove the _INTEL suffix here since we don't need to differentiate based on vendor for UR queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @kbenzie
I changed the enum name to "UR_BFLOAT_CONVERSIONS_NATIVE".
Thanks very much.

Signed-off-by: jinge90 <[email protected]>
device, UR_DEVICE_INFO_ATOMIC_MEMORY_SCOPE_CAPABILITIES, flags);
}

ur_result_t GetDeviceBFloat16ConversionsNativeSupport(ur_device_handle_t device,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this helper isn't used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @aarongreig
I removed the unused helper.
Thanks very much.

@aarongreig
Copy link
Contributor

can you add a CTS test, it can be identical to this one

TEST_P(urDeviceGetInfoTest, SuccessUseNativeAssert) {
but obviously with the test name/enum switched

@jinge90
Copy link
Contributor Author

jinge90 commented Mar 31, 2025

can you add a CTS test, it can be identical to this one

TEST_P(urDeviceGetInfoTest, SuccessUseNativeAssert) {

but obviously with the test name/enum switched

Hi, @aarongreig
I added the test, could you help review again?
Thanks very much.

@jinge90 jinge90 requested a review from a team April 2, 2025 02:11
@jinge90
Copy link
Contributor Author

jinge90 commented Apr 2, 2025

Hi, @intel/llvm-gatekeepers
Could you help merge this patch?
Thanks very much.

@steffenlarsen steffenlarsen merged commit 9c1cb9c into intel:sycl Apr 2, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants